-
Notifications
You must be signed in to change notification settings - Fork 166
RSA encryption padding change from PKCS1Padding to OAEPWithSHA-256And… #834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| "("+ OLD_PKCS1_RSA_TRANSFORMATION +") for migration due to " + | ||
| "InvalidKeyException with OAEP."); | ||
| KeyStore.PrivateKeyEntry rsaKeyEntry = getRSAKeyEntry(); | ||
| Cipher rsaPkcs1Cipher = Cipher.getInstance(OLD_PKCS1_RSA_TRANSFORMATION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semgrep identified an issue in your code:
Weak cryptographic algorithm detected. The DES, 3DES, RC2, RC4, MD4, MD5, SHA1, BLOWFISH, Dual_EC_DRBG, and SHA1PRNG algorithms are considered insecure and should not be used. Using weak cryptographic algorithms can compromise the confidentiality and integrity of sensitive data. It is recommended to adopt only cryptographic features and algorithms offered by the Android platform that are internationally recognized as strong. It is also fundamental to ensure that the encryption parameters (crypto key, IV, etc.) are generated randomly using a cryptographically strong PRNG function. In addition, if it is needed to store an encryption parameter on the device, a secure storage mechanism like the Android KeyStore must be used.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by android_weak_cryptographic_algorithm.
You can view more details about this finding in the Semgrep AppSec Platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary digest used for the RSA-OAEP padding scheme is SHA-256, which is secure.
SHA1 is included only as a supported digest for the Mask Generation Function (MGF1), an internal component of OAEP.
This is a standard practice to ensure maximum compatibility across different Android OS versions and hardware security modules, as some implementations require SHA1 to be available for MGF1.
| Log.d(TAG, "Attempting to decrypt AES key with PKCS1Padding ("+ | ||
| OLD_PKCS1_RSA_TRANSFORMATION +") for migration."); | ||
| KeyStore.PrivateKeyEntry rsaKeyEntry = getRSAKeyEntry(); | ||
| Cipher rsaPkcs1Cipher = Cipher.getInstance(OLD_PKCS1_RSA_TRANSFORMATION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weak cryptographic algorithm detected. The DES, 3DES, RC2, RC4, MD4, MD5, SHA1, BLOWFISH, Dual_EC_DRBG, and SHA1PRNG algorithms are considered insecure and should not be used. Using weak cryptographic algorithms can compromise the confidentiality and integrity of sensitive data. It is recommended to adopt only cryptographic features and algorithms offered by the Android platform that are internationally recognized as strong. It is also fundamental to ensure that the encryption parameters (crypto key, IV, etc.) are generated randomly using a cryptographically strong PRNG function. In addition, if it is needed to store an encryption parameter on the device, a secure storage mechanism like the Android KeyStore must be used.
🧼 Fixed in commit 715bcc8 🧼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary digest used for the RSA-OAEP padding scheme is SHA-256, which is secure.
SHA1 is included only as a supported digest for the Mask Generation Function (MGF1), an internal component of OAEP.
This is a standard practice to ensure maximum compatibility across different Android OS versions and hardware security modules, as some implementations require SHA1 to be available for MGF1.
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the RSA encryption padding from PKCS1Padding to OAEPWithSHA-256AndMGF1Padding for improved security. The change includes backward compatibility handling to migrate existing PKCS1-encrypted AES keys to the new OAEP format.
- Changes RSA transformation from PKCS1Padding to OAEPWithSHA-256AndMGF1Padding
- Increases RSA key size from 2048 to 4096 bits
- Implements migration logic for existing PKCS1-encrypted AES keys to maintain compatibility
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| CryptoUtil.java | Updates RSA transformation, key size, and implements migration logic for PKCS1 to OAEP transition |
| CryptoUtilTest.java | Refactors tests to support new RSA transformation and adds migration test coverage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Show resolved
Hide resolved
| storage.store(KEY_ALIAS, newEncodedEncryptedAES); | ||
| storage.remove(OLD_KEY_ALIAS); | ||
| return decryptedAESKey; | ||
| } catch (Exception e) { |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching generic Exception is too broad and may hide specific error conditions. Consider catching specific exceptions like BadPaddingException, IllegalBlockSizeException, NoSuchAlgorithmException, etc., to handle different failure scenarios appropriately.
| } catch (Exception e) { | |
| } catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidKeyException | BadPaddingException | IllegalBlockSizeException | KeyStoreException | UnrecoverableEntryException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made new changes on 17 dec will check on copilot review on it.
| Log.e(TAG, "Error while creating the AES key.", e); | ||
| Log.e(TAG, "Error while creating the new AES key.", e); | ||
| throw new IncompatibleDeviceException(e); | ||
| } catch (Exception e) { |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching generic Exception is too broad. This catch block should handle specific exceptions that can occur during RSA encryption or storage operations, such as CryptoException or IncompatibleDeviceException.
| } catch (Exception e) { | |
| } catch (InvalidKeyException | |
| | NoSuchPaddingException | |
| | IllegalBlockSizeException | |
| | BadPaddingException | |
| | KeyStoreException | |
| | UnrecoverableEntryException | |
| | CertificateException | |
| | IOException | |
| | ProviderException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made new changes on 17 dec will check on copilot review on it.
| // https://developer.android.com/reference/javax/crypto/Cipher.html | ||
| @SuppressWarnings("SpellCheckingInspection") | ||
| private static final String AES_TRANSFORMATION = "AES/GCM/NOPADDING"; | ||
| private static final String OLD_PKCS1_RSA_TRANSFORMATION = "RSA/ECB/PKCS1Padding"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment to state why OLD_PKCS1_RSA_TRANSFORMATION is being used
| .setKeySize(RSA_KEY_SIZE) | ||
| .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1) | ||
| .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_OAEP) | ||
| .setDigests(KeyProperties.DIGEST_SHA256, KeyProperties.DIGEST_SHA1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope this setDigest addition was also reviewed by security ?
|
@utkrishtsahu The current implementation doesn't handle migration for existing users token . Please handle that and ensure it doesn't break |
…gration logic for existing user
212b04c to
d9bce21
Compare
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Fixed
Show fixed
Hide fixed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Dismissed
Show dismissed
Hide dismissed
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java:391
- String concatenation using += in a loop can be inefficient. While this particular loop is expected to be short (traversing the exception chain), it's better practice to use a StringBuilder for string concatenation. This improves performance and is more maintainable if the loop grows longer in the future.
throw new IncompatibleDeviceException(e);
} catch (IllegalBlockSizeException | BadPaddingException e) {
/*
* They really should not be thrown at all since padding is requested in the transformation.
* Delete the AES keys since those originated the input.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!TextUtils.isEmpty(encodedEncryptedAES)) { | ||
| byte[] encryptedAESBytes = Base64.decode(encodedEncryptedAES, Base64.DEFAULT); | ||
| try { | ||
| return RSADecrypt(encryptedAESBytes); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation of the decrypted AES key length after PKCS1 decryption. The original getAESKey implementation validated that the AES key has the expected length (32 bytes for 256-bit AES) before returning it. Without this validation, a corrupted or malformed legacy key could be migrated and cause issues later. Consider validating that decryptedAESKey.length equals AES_KEY_SIZE / 8 before proceeding with re-encryption.
| return RSADecrypt(encryptedAESBytes); | |
| byte[] decryptedAESKey = RSADecrypt(encryptedAESBytes); | |
| // Validate that the decrypted AES key has the expected length (e.g. 32 bytes for 256-bit AES) | |
| if (decryptedAESKey == null || decryptedAESKey.length != AES_KEY_SIZE / 8) { | |
| // Treat this as corrupted key material: clean up and signal an error | |
| deleteRSAKeys(); | |
| deleteAESKeys(); | |
| throw new CryptoException("The RSA decrypted AES key has an unexpected length."); | |
| } | |
| return decryptedAESKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code already throws exceptions on decryption failures. If additional validation is desired, it should be addressed in a separate PR to avoid scope creep and maintain backward compatibility
| NoSuchPaddingException | InvalidKeyException | | ||
| IllegalBlockSizeException | BadPaddingException ex) { | ||
| Log.e(TAG, "Could not migrate. A new key will be generated.", ex); | ||
| deleteRSAKeys(); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching a generic Exception here is overly broad and could hide unexpected issues. The catch block swallows all exceptions (including potential runtime exceptions) and simply generates a new key, which could mask serious bugs. Consider catching only the specific checked exceptions that can be thrown by the operations inside the try block (NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException, CryptoException, KeyStoreException, CertificateException, IOException, UnrecoverableEntryException) to maintain better error visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception handling is in the OLD_KEY_ALIAS migration path (lines 464-467), which is unrelated to the PKCS1→OAEP security migration that this PR addresses. The generic Exception catch block existed in the original code and was not modified by this PR.
| rsaKeyEntry.getPrivateKey() | ||
| ); | ||
|
|
||
| byte[] encryptedAESWithOAEP = RSAEncrypt(decryptedAESKey); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catching a generic Exception here is overly broad and could mask programming errors or unexpected runtime exceptions. The outer catch for the key generation logic should only catch the specific exceptions that can be thrown. Since this is a new key generation path, catching Exception and wrapping it in CryptoException reduces visibility into actual failures. Consider catching only IncompatibleDeviceException and CryptoException that might propagate from RSAEncrypt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generic Exception catch block existed in the original code and was not modified by this PR. While catching specific exceptions would improve code quality, changing this would expand the scope beyond the security fix.
| when(keyStore.getEntry(eq(KEY_ALIAS), nullable(KeyStore.ProtectionParameter.class))) | ||
| .thenReturn(mockKeyEntry); | ||
|
|
||
| byte[] aesKeyBytes = new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test uses a 16-byte AES key, but AES-256 requires a 32-byte key (AES_KEY_SIZE = 256 bits = 32 bytes). The implementation should validate the decrypted AES key length to ensure it matches the expected 32 bytes. Using a 16-byte key in the test doesn't properly validate that the migration logic would reject incorrectly-sized keys. Consider using a properly-sized 32-byte key in the test to match the actual AES-256 specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests use 16-byte AES keys, which is consistent with the mocking pattern in this test file. All tests in CryptoUtilTest.java use dummy key sizes (most use 3-byte keys, migration tests use 16-byte keys) because all encryption operations are fully mocked - no real cryptographic operations occur.
The tests validate BEHAVIOR (migration flow, storage updates, method calls) not actual encryption. Since RSADecrypt/RSAEncrypt are mocked, the key size doesn't affect test validity.
Changing to 32-byte keys would be cosmetic and inconsistent with existing test patterns. No fix needed.
| String newEncodedEncryptedAES = new String( | ||
| Base64.encode(encryptedAESWithOAEP, Base64.DEFAULT), | ||
| StandardCharsets.UTF_8 | ||
| ); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation of the decrypted AES key length after PKCS1 decryption from OLD_KEY_ALIAS. The original getAESKey implementation validated that the AES key has the expected length (32 bytes for 256-bit AES) before returning it. Without this validation, a corrupted or malformed legacy key could be migrated. Consider validating that decryptedAESKey.length equals AES_KEY_SIZE / 8 before proceeding with re-encryption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While adding AES key length validation would be good defensive programming, it's beyond the scope of this PR, which focuses specifically on migrating from PKCS1 to OAEP padding for the security fix.
The existing error handling already addresses corrupted keys: if the decrypted AES key is malformed, subsequent encryption operations will fail and trigger the cleanup path that deletes corrupted keys and regenerates them. This behavior is consistent with the pre-migration code.
| public void shouldDetectAndMigratePKCS1KeyToOAEP() throws Exception { | ||
| CryptoUtil cryptoUtil = newCryptoUtilSpy(); | ||
|
|
||
| byte[] aesKeyBytes = new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test uses a 16-byte AES key (line 1701), but AES-256 requires a 32-byte key (AES_KEY_SIZE = 256 bits = 32 bytes). The implementation should validate the decrypted AES key length to ensure it matches the expected 32 bytes. Using a 16-byte key in the test doesn't properly validate that the migration logic would reject incorrectly-sized keys. Consider using a properly-sized 32-byte key in the test to match the actual AES-256 specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests use 16-byte AES keys, which is consistent with the mocking pattern in this test file. All tests in CryptoUtilTest.java use dummy key sizes (most use 3-byte keys, migration tests use 16-byte keys) because all encryption operations are fully mocked - no real cryptographic operations occur.
The tests validate BEHAVIOR (migration flow, storage updates, method calls) not actual encryption. Since RSADecrypt/RSAEncrypt are mocked, the key size doesn't affect test validity.
Changing to 32-byte keys would be cosmetic and inconsistent with existing test patterns. No fix needed.
| public void shouldUseOAEPDirectlyForNewUsers() throws Exception { | ||
| CryptoUtil cryptoUtil = newCryptoUtilSpy(); | ||
|
|
||
| byte[] aesKeyBytes = new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test uses a 16-byte AES key, but AES-256 requires a 32-byte key (AES_KEY_SIZE = 256 bits = 32 bytes). The implementation should validate the decrypted AES key length to ensure it matches the expected 32 bytes. Using a 16-byte key in the test doesn't properly validate that the migration logic would reject incorrectly-sized keys. Consider using a properly-sized 32-byte key in the test to match the actual AES-256 specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests use 16-byte AES keys, which is consistent with the mocking pattern in this test file. All tests in CryptoUtilTest.java use dummy key sizes (most use 3-byte keys, migration tests use 16-byte keys) because all encryption operations are fully mocked - no real cryptographic operations occur.
The tests validate BEHAVIOR (migration flow, storage updates, method calls) not actual encryption. Since RSADecrypt/RSAEncrypt are mocked, the key size doesn't affect test validity.
Changing to 32-byte keys would be cosmetic and inconsistent with existing test patterns. No fix needed.
| cause = cause.getCause(); | ||
| } | ||
|
|
||
| if (fullMessage.contains("Incompatible padding mode") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String comparison for checking the old padding doesn't looks like a good method as it might return inconsistent behaviour. Would suggest to do some other approach to differentiate
| try { | ||
| return RSADecrypt(encryptedAESBytes); | ||
| } catch (IncompatibleDeviceException e) { | ||
| String fullMessage = e.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire code block can be refactored to make it more readable
| if (existingAES != null && existingAES.length == aesExpectedLengthInBytes) { | ||
| //Key exists and has the right size | ||
| return existingAES; | ||
| String encodedOldAES = storage.retrieveString(OLD_KEY_ALIAS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to handle both the OLD_KEY_ALIAS and KEY_ALIAS in separate block here ? Can't a single block like in the original work ?
RSA encryption padding change from PKCS1Padding to OAEPWithSHA-256And
Changes
Updated padding for RSA encryption from PKCS1Padding to OAEPWithSHA-256And also checked for migration for the same.
References
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
[X ] This change adds unit test coverage
[X ] This change adds integration test coverage
[ X] This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors